-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add better error checking and message #5911 #5311 #6794
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
1 similar comment
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending a PR!
Could you run yarn eslint packages/jest-jasmine2/src/jasmine/Env.js --fix
to remove the unrelated whitespace changes?
This also needs a test and a changelog entry 🙂
message | ||
} = checkIsError(error); | ||
|
||
const runnable = currentRunnable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can move this above checkIsError
as that's not necessary if we throw here.
@aaronabramov does circus behave correctly here already? |
i think so! but i'm not 100% sure :) i remember we did some handling there while we were in LON |
@SimenB Will do. |
Hey. I'm going to cancel this. I've tried quite a bit to get the test running |
@jleach sorry about the slow response. What do you mean missing packages? Have you followed https://github.com/facebook/jest/blob/master/CONTRIBUTING.md#workflow-and-pull-requests? |
Best way to test it is probably to add an e2e test if you have a good reproduction. A quick reproduction I threw together: test('a failing test', done => {
setTimeout(() => done('fail async'), 5);
done();
}); You can look at other tests on the With the changes in this PR, I get the following error from jasmine: @aaronabramov not sure if I think Circus does this correctly. Should we try to throw an error if stuff is done after the test is complete? |
@jleach wanna pick this up again? 🙂 |
@SimenB You may already be familiar with this, but I've found "apply mail" (with hub) is very helpful when a PR is mostly there but just needs a CHANGELOG and so on. |
@nathany that article is a bit dated - maintainers can push to PRs (so I can add the changelog on github), and we squash merge for the clean history. However, the issue with this PR is mostly that it does not have a test. We also would have to make sure circus behaves correctly 🙂 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
I ran into the bug described in #5911 #5311 (et al) and found it tough to figure out what was causing the problem.
Test plan
The command:
NODE_ENV=test npx jest -I __tests__
Now triggers the error case:
(node:1915) UnhandledPromiseRejectionWarning: Error: Caught error after test environment was torn down